Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Capture git export-subst strings in version.sh for 'git archive' use. #55353

Merged
merged 1 commit into from
Nov 10, 2017
Merged

Capture git export-subst strings in version.sh for 'git archive' use. #55353

merged 1 commit into from
Nov 10, 2017

Conversation

david-mcmahon
Copy link
Contributor

@david-mcmahon david-mcmahon commented Nov 8, 2017 •
edited

Eliminate the need to update pkg/version/base.go on release branch tagging.

This excellent solution brought to you by @ixdy.

ref #16815 (somewhat related)
cc @javier-b-perez

@david-mcmahon david-mcmahon added area/build-release area/release-eng Issues or PRs related to the Release Engineering subproject release-note-none Denotes a PR that doesn't merit a release note. sig/release Categorizes an issue or PR as relevant to SIG Release. labels Nov 8, 2017
@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 8, 2017
@david-mcmahon david-mcmahon added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 8, 2017
@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 8, 2017
@david-mcmahon
Copy link
Contributor Author

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 8, 2017
ixdy
ixdy reviewed Nov 8, 2017
View reviewed changes
hack/lib/version.sh Outdated
if [[ '$Format:%%$' == "%" ]]; then
KUBE_GIT_COMMIT='$Format:%H$'
KUBE_GIT_TREE_STATE="not a git tree"
if [[ '$Format:%D$' =~ tag:\ (v[^ ]+) ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may want to leave a comment on why this works, something like
"If the archive was created at a git tag vX.Y.Z, then the ref names will include tag: vX.Y.Z"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Done.

@ixdy
Copy link
Member

ixdy commented Nov 8, 2017

/lgtm
/hack

I still can't believe this actually works. :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2017
@david-mcmahon
Copy link
Contributor Author

Well to be fair, it's already being done in base.go to some degree and you just put the final bow on it and wrapped it up. :) Can't wait to delete the code in k/r!

ixdy
ixdy reviewed Nov 8, 2017
View reviewed changes
hack/lib/version.sh Outdated
# we likely don't have a git tree, but these magic values may be filled in.
if [[ '$Format:%%$' == "%" ]]; then
KUBE_GIT_COMMIT='$Format:%H$'
KUBE_GIT_TREE_STATE="not a git tree"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder if this should be "git archive" instead of "not a git tree"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precedence, though we could change it. Maybe in both locations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we want to change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To git archive or empty?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git archive

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@ixdy
Copy link
Member

ixdy commented Nov 8, 2017

we can also probably remove the logic in pkg/version/base.go and just keep it here.

@ixdy
Copy link
Member

ixdy commented Nov 8, 2017

well, maybe not. we're not guaranteed that GIT_VERSION will get set here.

@david-mcmahon
Copy link
Contributor Author

@ixdy Do you mean move the other $Format's over to the script? Any reason NOT to do that?

@ixdy
Copy link
Member

ixdy commented Nov 9, 2017

pkg/version/base.go has

  gitVersion   = "v0.0.0-master+$Format:%h$"

which will only get overridden by the hack/lib/version.sh hacks if we git archive at a version tag. right now it's mostly useless, though it still has a git sha. I'm not sure whether we want to remove the sha or not.

re: changing not a git tree in base.go: it'd be weird to put a default value of git archive there. I guess we could just set it to empty?

@david-mcmahon
Copy link
Contributor Author

@ixdy set it to empty? I'll buy that.

WRT gitVersion's git sha, I don't feel strongly either way. @zmerlynn?

@david-mcmahon
Copy link
Contributor Author

Is there any reason why staging/src/k8s.io/client-go/pkg/version is not a symlink to pkg/version (or at least some of the files in there)?

david-mcmahon added a commit to david-mcmahon/release that referenced this pull request Nov 13, 2017
@david-mcmahon
No longer need to update base.go via rev_version_base().
e31cc7a 
Ref kubernetes/kubernetes#55353.
k8s-github-robot pushed a commit that referenced this pull request Nov 13, 2017
Merge pull request #55494 from david-mcmahon/automated-cherry-pick-of…
fd4d97d 
…-#55353-upstream-release-1.7

Automatic merge from submit-queue.

Automated cherry pick of #55353

Cherry pick of #55353 on release-1.7.

#55353: Capture git export-subst strings in version.sh for 'git
@BenTheElder BenTheElder mentioned this pull request Nov 17, 2017
Closed
@david-mcmahon david-mcmahon mentioned this pull request Nov 21, 2017
Merged
@david-mcmahon david-mcmahon mentioned this pull request Nov 21, 2017
Merged
@david-mcmahon david-mcmahon mentioned this pull request Nov 21, 2017
Merged
david-mcmahon added a commit to david-mcmahon/kubernetes that referenced this pull request Nov 21, 2017
@david-mcmahon
Use v0.0.0 gitVersion on branches in support of new .gitattributes so…
f6f8823 
…lution.

Update bad link as well.
Prefer master branch untyped values.

ref kubernetes#55353.
david-mcmahon added a commit to david-mcmahon/kubernetes that referenced this pull request Nov 21, 2017
@david-mcmahon
Use v0.0.0 gitVersion on branches in support of new .gitattributes so…
d51569d 
…lution.

Update bad link as well.
Prefer master branch untyped values.

ref kubernetes#55353.
david-mcmahon added a commit to david-mcmahon/kubernetes that referenced this pull request Nov 21, 2017
@david-mcmahon
Use v0.0.0 gitVersion on branches in support of new .gitattributes so…
5128e3f 
…lution.

Update bad link as well.
Prefer master branch untyped values.

ref kubernetes#55353.
@sttts
Copy link
Contributor

sttts commented Nov 21, 2017

Is there any reason why staging/src/k8s.io/client-go/pkg/version is not a symlink to pkg/version (or at least some of the files in there)?

We export client-go. A symlink will become invalid when client-go is in its own repo.

@sttts
Copy link
Contributor

sttts commented Nov 21, 2017

Has this been tested with client-go and especially the export of client-go?

@david-mcmahon david-mcmahon mentioned this pull request Nov 21, 2017
Closed
@david-mcmahon
Copy link
Contributor Author

I tested this by ensuring the scripts and go files updated at export, but nothing beyond that. Is there a specific test for client-go we can verify?

@sttts
Copy link
Contributor

sttts commented Nov 22, 2017

@david-mcmahon you find up-to-date published branches at https://github.com/sttts/client-go. My main question is whether somebody using these library will see the right version when accessing pkg/version.

@ixdy
Copy link
Member

ixdy commented Nov 22, 2017

@sttts is client-go published from release branches or from master?

@ixdy ixdy mentioned this pull request Nov 22, 2017
Closed
david-mcmahon added a commit to david-mcmahon/kubernetes that referenced this pull request Nov 22, 2017
@david-mcmahon
Use v0.0.0 gitVersion on branches in support of new .gitattributes so…
e576bfa 
…lution.

ref kubernetes#55353.
@sttts
Copy link
Contributor

sttts commented Nov 24, 2017

@ixdy both. The release version of client-go comes from the release branch of kube.

k8s-github-robot pushed a commit that referenced this pull request Dec 4, 2017
Merge pull request #56107 from david-mcmahon/basego-clear
2eb9015 
Automatic merge from submit-queue.

Use v0.0.0 gitVersion on branches in support of new .gitattributes solution.

Update bad link as well.
Prefer master branch untyped values.

ref #55353.
k8s-github-robot pushed a commit that referenced this pull request Dec 21, 2017
Merge pull request #56111 from david-mcmahon/basego-17
7b3a0f1 
Automatic merge from submit-queue.

Use v0.0.0 gitVersion on branches in support of new .gitattributes solution.

Update bad link as well.
Prefer master branch untyped values.

ref #55353.
@ravilr ravilr mentioned this pull request Mar 12, 2018
Closed
This was referenced Feb 28, 2019
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@ixdy ixdy ixdy left review comments

@javier-b-perez javier-b-perez javier-b-perez left review comments

Assignees

@lavalamp lavalamp

@zmerlynn zmerlynn

@ixdy ixdy

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/build-release area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/release Categorizes an issue or PR as relevant to SIG Release. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet